include version requirements in path for resolver error messages #5452

Eh2406 · 2018-05-01T20:09:11Z

This is a follow up to #5428 (comment). It changes the error messages from:

Lines 1356 to 1358 in 0b530c3

    
             previously selected package `bad v1.0.0` 
        
               ... which is depended on by `bar v0.1.0` 
        
               ... which is depended on by `incompatible_dependencies v0.0.1 ([..])`

to:

https://github.com/rust-lang/cargo/blob/499340a81624eaef77d969a3e9fd2cf94f933a2e/tests/testsuite/build.rs#L1356-L1358

Sorry for the big diff, this includes a cargo +stable fmt commit before my changes.

rust-highfive · 2018-05-01T20:09:15Z

r? @matklad

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2018-05-02T03:03:43Z

This looks pretty awesome, a great idea!

I wonder though if we may want to tweak the wording slightly? It may be slightly redundant to have the name of the package like foo = "0.1" because it's repeated on every line, and I think it's also not necessarily guaranteed to match what's written in the manifest as well.

It may also be worth avoiding printing a * dependency requirement for things like git/path dependencies, but I'm not acutally sure how that could cause a conflict so it may not be too relevant!

bors · 2018-05-02T14:40:32Z

☔ The latest upstream changes (presumably #5432) made this pull request unmergeable. Please resolve the merge conflicts.

Eh2406 · 2018-05-02T18:26:40Z

I am open to changing the wording.

Repeating the name is definitely redundant, what frazing is unambiguous and clearer?

You are correct the semver::Version will canonicalized the requirement. If the manifest has bar = "0.1.0" the output will be bar = "^0.1.0". I don't think we keep the pre parsed string anyware. How big a deal do you think this is?

Is there an easy way to distinguish git/path dependencies, so as to remove "*" that is nowhere in the manifest?

A fix for CI and a rebase on its way.

alexcrichton · 2018-05-02T18:40:48Z

Oh sorry when I meant "written in the manifest" I meant moreso foo = { version = "0.1" } vs foo = "0.1" (maybe sometimes with optional = true). I think canonicalizing "0.1" to "^0.1" is fine to do in the error message.

Oh for sure yeah, the SourceId of the dependency will return true for is_path or is_git, and you can also use !is_registry as well for printing the vesion perhaps

Eh2406 · 2018-05-03T17:52:54Z

Rebase took a couple of tries, but doing it manually worked.

I removed the redundant use of name. Do you have a less verbose/stilted way of saying this? And, a preferred wording for git/path?

alexcrichton · 2018-05-03T18:49:34Z

Nah this looks reasonable to me! If the version requirement is * (or if it's a git/path dep) I'd be fine just omitting the 'for this version requirement' text as well (aka leave as-is with today)

@matklad wanted to cc you as well, you think the new output is alright?

bors · 2018-05-03T19:40:21Z

☔ The latest upstream changes (presumably #5473) made this pull request unmergeable. Please resolve the merge conflicts.

matklad · 2018-05-03T19:56:40Z

I definitely like the information that is presented, however all these ... selected to fulfill are hard to parse for me. I would say that there are two problems with this presentation:

package and it's version requirement are on adjacent lines, so you have to read this stuff diagonally to make sense
we try hard to present a tabular data as a single English sentence, and that feels clunky.

As alternative, what about this more line oriented and tabular presentation:

BEFORE:
error: failed to select a version for `bad`.
    ... required by package `incompatible_dependencies v0.0.1 ([..])`
versions that meet the requirements `>= 1.0.1, <= 2.0.0` are: 2.0.0, 1.0.1
all possible versions conflict with previously selected packages.
  previously selected package `bad v2.0.1`
    ... selected to fulfill the requirement \">= 2.0.1\" from package `baz v0.1.0`
    ... selected to fulfill the requirement \"^0.1.0\" from package `incompatible_dependencies v0.0.1 ([..])`
  previously selected package `bad v1.0.0`
    ... selected to fulfill the requirement \"= 1.0.0\" from package `bar v0.1.0`
    ... selected to fulfill the requirement \"^0.1.0\" from package `incompatible_dependencies v0.0.1 ([..])`
failed to select a version for `bad` which could resolve this conflict

AFTER:
error: failed to select a version for `bad`.
   `bad >= 1.0.1, <= 2.0.0` required by `incompatible_dependencies v0.0.1 ([..])`

versions that meet the requirements `bad >= 1.0.1, <= 2.0.0` are: 2.0.0, 1.0.1
all possible versions conflict with previously selected packages.

previously selected package `bad v2.0.1`
  `bad >= 2.0.1` required by `baz v0.1.0`, picked  `bad v2.0.1`
  `baz ^0.1.0` required by `incompatible_dependencies v0.0.1 ([..])`, picked `baz v0.1.0`

previously selected package `bad v1.0.0`
  `bad  =1.0.0` required by `bar v0.1.0`, picked  `bad v1.0.0`
  `bar ^0.1.0` required by `incompatible_dependencies v0.0.1 ([..])`,  picked `bar v0.1.0`

failed to select a version for `bad` which could resolve this conflict",

I am not really attached to this format, but I like some of its properties:

it purposefully duplicates version info, so as to make each line complete in isolation
version requirements (which are the underling cause of the conflict) are in the first column.
simpler and shorter English words
blank lines ❤️ ❤️ ❤️

matklad · 2018-05-03T19:58:15Z

As this is hard to rebase, I suggest to merge this ASAP, and tweak rendering details in subsequent PR though, if we want to tweak them (I actually quite like the current rendering, I just think it could be even better).

Eh2406 · 2018-05-06T01:01:00Z

as pointed out in #5484, we should mention that precise requirements may be caused by lock files and suggest using update or generate lock file to fix it.

alexcrichton · 2018-07-05T19:17:50Z

Oh dear it looks like this may have fallen off the radar by accident! @Eh2406 was there any other blockers on this or did you feel this was good to go?

Eh2406 · 2018-07-05T19:44:45Z

It fell off my radar as well. What was left to do:

Agree on desired wording.
Special case git/path dependencies without breaking the structure of the output.
Special case pind by a lock file and not a "=1.1.2" in a dependency. Also add a "note: try cargo update" to the end.

alexcrichton · 2018-07-06T21:29:38Z

Ah ok, no worries!

bors · 2018-07-16T20:40:50Z

☔ The latest upstream changes (presumably #5691) made this pull request unmergeable. Please resolve the merge conflicts.

alexcrichton · 2018-07-31T19:06:38Z

Ok I'm gonna close this for now due to inactivity, but we can of course reopen and/or resubmit!

Improve resolver message to include dependency requirements Resolves #6199. Thanks for previous efforts: #5452, #6374, #6665, which are great but somehow outdated, so I tweak them and create this PR. This will also be obsolete if we ship pubgrub-rs with cargo in the future 😃 But before that happens, IMO these changes are still helpful. --- This PR changes the resolver error message from https://github.com/rust-lang/cargo/blob/216f915c46b8ada2323423d049314ba18247ef95/tests/testsuite/build.rs#L1104-L1106 to https://github.com/rust-lang/cargo/blob/0afd40b4de17a5c45145a0762beb4ef001720fe1/tests/testsuite/build.rs#L1104-L1106 Also provide different message for different source kinds, such like: https://github.com/rust-lang/cargo/blob/0afd40b4de17a5c45145a0762beb4ef001720fe1/tests/testsuite/build.rs#L2810-L2812 ## TODO? From #5452 (comment), there shall be at least one task left behind: > 3. Special case pind by a lock file and not a `"=1.1.2"` in a dependency. Also add a "note: try cargo update" to the end. In this PR, `validate_links` also faces this issue that a dependency requirement is locked into a precise version `=0.1.0`. https://github.com/rust-lang/cargo/blob/a5f8bc94f5d38539dd127f735ea4d3a515c230fd/tests/testsuite/build_script.rs#L1002-L1004 I am uncertain about how to resolve this. Besides the function`validate_links`, is this problem really a thing that may happen? If not, since `validate_links` only handles old validation logic, it may be ok to drop the commit a5f8bc9 and leave it as is.

rust-highfive assigned matklad May 1, 2018

Eh2406 force-pushed the master branch 2 times, most recently from d448829 to aff01ae Compare May 3, 2018 17:43

This comment has been minimized.

Sign in to view

Eh2406 force-pushed the master branch 2 times, most recently from 119bc0f to d2ed993 Compare May 10, 2018 18:06

Eh2406 added 4 commits May 14, 2018 13:21

cargo +stable fmt

fa3ab49

include version requirements in path for resolver error messages

3d68ca0

clean up args

48251c6

remove redundant use of name

598f124

Eh2406 force-pushed the master branch from d2ed993 to 598f124 Compare May 14, 2018 17:56

alexcrichton closed this Jul 31, 2018

Eh2406 mentioned this pull request Oct 2, 2018

Try to improve "version not found" error #6112

Merged

Eh2406 unassigned matklad Dec 5, 2018

Eh2406 mentioned this pull request Dec 5, 2018

resolve error messages #6374

Closed

spacekookie mentioned this pull request Feb 18, 2019

Improve resolve error messages #6665

Closed

weihanglo mentioned this pull request Aug 23, 2021

Improve resolver message to include dependency requirements #9827

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

include version requirements in path for resolver error messages #5452

include version requirements in path for resolver error messages #5452

Eh2406 commented May 1, 2018

rust-highfive commented May 1, 2018

alexcrichton commented May 2, 2018

bors commented May 2, 2018

Eh2406 commented May 2, 2018

alexcrichton commented May 2, 2018

Eh2406 commented May 3, 2018

alexcrichton commented May 3, 2018

bors commented May 3, 2018

matklad commented May 3, 2018

matklad commented May 3, 2018

This comment has been minimized.

This comment has been minimized.

Eh2406 commented May 6, 2018

alexcrichton commented Jul 5, 2018

Eh2406 commented Jul 5, 2018

alexcrichton commented Jul 6, 2018

bors commented Jul 16, 2018

alexcrichton commented Jul 31, 2018

	previously selected package `bad v1.0.0`
	... which is depended on by `bar v0.1.0`
	... which is depended on by `incompatible_dependencies v0.0.1 ([..])`

include version requirements in path for resolver error messages #5452

include version requirements in path for resolver error messages #5452

Conversation

Eh2406 commented May 1, 2018

rust-highfive commented May 1, 2018

alexcrichton commented May 2, 2018

bors commented May 2, 2018

Eh2406 commented May 2, 2018

alexcrichton commented May 2, 2018

Eh2406 commented May 3, 2018

alexcrichton commented May 3, 2018

bors commented May 3, 2018

matklad commented May 3, 2018

matklad commented May 3, 2018

This comment has been minimized.

This comment has been minimized.

Eh2406 commented May 6, 2018

alexcrichton commented Jul 5, 2018

Eh2406 commented Jul 5, 2018

alexcrichton commented Jul 6, 2018

bors commented Jul 16, 2018

alexcrichton commented Jul 31, 2018